Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

chore: deprecate pylint in favor of ruff #31262

Merged
merged 5 commits into from
Dec 13, 2024
Merged

chore: deprecate pylint in favor of ruff #31262

merged 5 commits into from
Dec 13, 2024
+22 −455

Conversation

mistercrunch
Copy link
Member

@mistercrunch mistercrunch commented Dec 2, 2024 •
edited
Loading

This could be slightly controversial, but submitted as a DRAFT for discussion. After thinking about it, I think we should just move forward with this. Shouldn't be controversial. File under simplify/accelerate builds/devex/tooling.

The case for pylint deprecation:

  • mostly covered by ruff: what matters most in pylint is already re-implemented
  • pylint is SLOW, ruff is a million times faster - pre-commit / pylint is one of our longest, most expensive CI step
  • rules are currently scattered in different places and can conflict at times
  • checked in with the Airflow community, they went all in on ruff, deprecated pylint and are extremely happy with the switch

Very relevant is this link that explains what pylint features are covered or not by ruff:
astral-sh/ruff#970

Essentially we can simplify & speed things up here..

If we agree on the direction, the plan is to follow up and enable all the rules, apply ruff check --fix and ruff check --add-no-qa, which will enable the set of rules commented now, fix the obvious, and add a # noqa to the lines that are not immediately fixable.

@mistercrunch
chore: deprecarte pylint in favor of ruff
89ec710 
This could be slightly controversial, but submitted as a DRAFT for discussion.

The case for pylint deprecation:
- mostly covered by ruff: what matters most in pylint is already re-implemented
- pylint is SLOW, ruff is a million times faster - pre-commit / pylint is one of our longest, most expensive CI step
- rules are currently scattered in different places and can conflict at times

Very relevant is this link that explains what pylint features are covered or not by ruff:
astral-sh/ruff#970

If we agree on the direction, the plan is to follow up and enable all the rules, apply `ruff check --fix` and `ruff check --add-no-qa`

Simplify.
@github-actions github-actions bot added doc Namespace | Anything related to documentation preset-io labels Dec 2, 2024
@mistercrunch mistercrunch marked this pull request as ready for review December 3, 2024 01:38
@michael-s-molina
Copy link
Member

michael-s-molina commented Dec 3, 2024 •
edited
Loading

@mistercrunch Can we reserve this for the 5.0 breaking window? We'll gather proposals for 5.0 next week. I'll send an email and you could include this. I know that this will break our internal release so using a major release is safer.

@michael-s-molina michael-s-molina added the risk:breaking-change Issues or PRs that will introduce breaking changes label Dec 3, 2024
@mistercrunch
Copy link
Member Author

Can we reserve this for the 5.0 breaking window

When is this scheduled? Thinking about it though, switching dev tools shouldn't have any impacts on UX/interfaces. From my understanding semver is mostly interface oriented. I'm favoring moving fast on this while I have momentum on improving devex. Though it may have impact on mergeability of cherries, so may be good to wait for that.

Also noting that when I enabling the pylint rules currently commented in my PR using ruff, it finds hundreds lint issues, meaning turning on ruff should up code quality moving forward (my plan is to enable these rules, run --fix, and --add-noqa).

@mistercrunch
Copy link
Member Author

@sadpandajoe curious on your thoughts about timing, for context the follow up PR would touch the backend with minor linting tweaks, maybe 300 LoC across the backend.

@sadpandajoe
Copy link
Member

@sadpandajoe curious on your thoughts about timing, for context the follow up PR would touch the backend with minor linting tweaks, maybe 300 LoC across the backend.

@mistercrunch I believe the breaking change window is opening soon. I don't think it'll affect Preset but sounds like it might have an affect on AirBnb?

@mistercrunch mistercrunch changed the title chore: deprecarte pylint in favor of ruff chore: deprecate pylint in favor of ruff Dec 3, 2024
@mistercrunch mistercrunch changed the title chore: deprecate pylint in favor of ruff chore: deprecate pylint in favor of ruff Dec 3, 2024
@mistercrunch mistercrunch added the v5.0 Label added by the release manager to track PRs to be included in the 5.0 branch label Dec 9, 2024
villebro
villebro approved these changes Dec 13, 2024
View reviewed changes
Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was hesitant at first, but given all the persistent unfixed critical issues on pylint, and the momentum ruff has, I agree this is the future right now. LGTM with a minor typo nit.

docs/docs/contributing/howtos.mdx Outdated Show resolved Hide resolved
@mistercrunch
Copy link
Member Author

@michael-s-molina do you still think we should hold back on the 5.0 process or can I just merge?

@villebro
Copy link
Member

Hmm I don't think this should necessarily be tied to the 5.0 breaking window 🤔 I say 🚢 it, but let's wait for @michael-s-molina to confirm.

@michael-s-molina
Copy link
Member

michael-s-molina commented Dec 13, 2024 •
edited
Loading

I'm ok with merging this now if you think it's not controversial. I would just ask to add something to UPDATING.md as there are orgs that extend Superset's code and are currently using pylint. I'll remove this PR from the 5.0 board.

@michael-s-molina michael-s-molina removed risk:breaking-change Issues or PRs that will introduce breaking changes v5.0 Label added by the release manager to track PRs to be included in the 5.0 branch labels Dec 13, 2024
@mistercrunch
Copy link
Member Author

Ok adding a note in UPDATING.md and mergin'! 🚢

@mistercrunch
Copy link
Member Author

mistercrunch commented Dec 13, 2024 •
edited
Loading

Ok, will merge and fast follow with another PR enabling more ruff rules (uncommenting what's here https://github.com/apache/superset/pull/31262/files#diff-50c86b7ed8ac2cf95bd48334961bf0530cdc77b5a56f852c5c61b89d735fd711R290-R302) that in some edge cases collided between ruff --fix and pylint (they disagreed on technicalities around enforcing/fixing certain rules - both takes were sane but not exactly the same in all cases)

@mistercrunch mistercrunch merged commit 4bccf36 into master Dec 13, 2024
35 checks passed
@mistercrunch mistercrunch deleted the rm_pylint branch December 13, 2024 20:53
@mistercrunch
Copy link
Member Author

Fast-follow here -> #31447

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@villebro villebro villebro approved these changes

@betodealmeida betodealmeida Awaiting requested review from betodealmeida

@eschutho eschutho Awaiting requested review from eschutho

@justinpark justinpark Awaiting requested review from justinpark

Assignees

@mistercrunch mistercrunch

Labels
doc Namespace | Anything related to documentation preset-io size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mistercrunch @michael-s-molina @sadpandajoe @villebro